Skip to content

Conversation

@Alenar
Copy link
Collaborator

@Alenar Alenar commented Nov 13, 2024

Content

Warning

This PR changes the certificate hashes computation when their signed entity type is CardanoImmutableFilesFull, the recompute-certificates-hash tool must be run in order to avoid breaking the chain.

This PR remove the network field from the CardanoDbBeacon struct. With the groundwork done in #2097 no additional adaptation was needed.

With this field removal some tools and services that used it only to produce CardanoDbBeacon were simplified:

  • signed entity config structure: used to provide conversion from timepoints to signed entities
  • epoch services from both the aggregator and signer: used to provide signed entity config struct

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update documentation website (if relevant)

Comments

Some network field still persist in serialized CardanoDbBeacon in database:

  • Aggregator:
    • open_message.beacon: existing network won't matter as they won't be mapped when read and since open messages are pruned regularly they will disappear by themself.
    • certificate.beacon: existing network won't matter as they won't be mapped when read, plus certificate hashes will be recomputed for all certificates that hold a CardanoDbBeacon meaning that all those certificates will be re-inserted in DB, eliminating the old network field.
    • signed_entity.beacon: for previously stored CardanoImmutableFilesFull entities the network will remain but should not matter as it won't be read when de-serialized.
    • signed_entity.artifact: for previously stored CardanoImmutableFilesFull entities the artifact a network field will remain in serialized Snapshot, this should not matter as it won't be read when de-serialized.
  • Signer:
    • signed_beacon.beacon: existing network won't matter as they won't be mapped when read and since signed beacon are pruned regularly they will disappear by themself.

Question: Should the database be cleaned of those remaining data with a migration ? Especially for signed_entities as those data won't be removed.

Issue(s)

Closes #1957

WARNING: this change invalidate hashes of certificate that with signed
entity type of type `CardanoImmutableFilesFull`, the
`recompute-certificates-hash` command must be run on aggregators in
order to keep their certificate chain valid.
This allow, by cascade, to remove the `network` of signer & aggregator
epoch services.
@Alenar Alenar self-assigned this Nov 13, 2024
@github-actions
Copy link

github-actions bot commented Nov 13, 2024

Test Results

    4 files  ±0     51 suites  ±0   11m 44s ⏱️ +8s
1 445 tests  - 4  1 445 ✅  - 4  0 💤 ±0  0 ❌ ±0 
1 656 runs   - 4  1 656 ✅  - 4  0 💤 ±0  0 ❌ ±0 

Results for commit 1dfedf1. ± Comparison against base commit e2fa1e0.

♻️ This comment has been updated with latest results.

by covering all cases (epoch + 1 > epoch was missing) and epoch priority
over immutable file number.
Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@Alenar Alenar force-pushed the djo/1957/remove_network_from_cardano_db_beacon branch from 2cae308 to 5860e91 Compare November 14, 2024 11:22
@Alenar Alenar force-pushed the djo/1957/remove_network_from_cardano_db_beacon branch from 5860e91 to 5a424cb Compare November 14, 2024 11:26
* mithril-persistence from `0.2.33` to `0.2.34`
* mithril-aggregator from `0.5.110` to `0.5.111`
* mithril-client-cli from `0.10.2` to `0.10.3`
* mithril-common from `0.4.86` to `0.4.87`
* mithril-signer from `0.2.212` to `0.2.213`
* mithril-end-to-end from `0.4.48` to `0.4.49`
@Alenar Alenar force-pushed the djo/1957/remove_network_from_cardano_db_beacon branch from 54bb930 to 1dfedf1 Compare November 14, 2024 11:46
@Alenar Alenar temporarily deployed to testing-preview November 14, 2024 11:55 — with GitHub Actions Inactive
@Alenar Alenar temporarily deployed to testing-sanchonet November 14, 2024 11:55 — with GitHub Actions Inactive
@Alenar Alenar merged commit f64b6e4 into main Nov 14, 2024
47 checks passed
@Alenar Alenar deleted the djo/1957/remove_network_from_cardano_db_beacon branch November 14, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove network field from CardanoDbBeacon

5 participants